Skip to content

refactor(cni): extract galactic-route as its own CNI chain plugin - #306

Open
privateip wants to merge 2 commits into
refactor/cni-chain-2-galactic-bgpfrom
refactor/cni-chain-3-galactic-route
Open

refactor(cni): extract galactic-route as its own CNI chain plugin#306
privateip wants to merge 2 commits into
refactor/cni-chain-2-galactic-bgpfrom
refactor/cni-chain-3-galactic-route

Conversation

@privateip

@privateip privateip commented Aug 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Stack (merge bottom to top):

Summary

Fourth branch in the CNI plugin-chain split stack (based on #305). galactic-route is now its own chained CNI plugin — invoked between the master plugin and galactic-bgp per conflist order — instead of termination-route installation inside galactic-cni/galactic-tap-cni. Unlike every other binary in the chain, it has zero Kubernetes dependency: no CRD reads or writes, no namespace.

What moved

internal/cniroute mirrors cniipam/cnibgp's shape: RunPlugin(), PluginConf{VPC, VPCAttachment, Terminations}. parseConf reuses config.CNIConfig for LogFile/LogLevel precedence but never resolves NodeName or Kubeconfig — nothing here talks to the API server.

cmdAdd installs each termination as a VRF route via the existing internal/cni/route package, deriving the host device name from (vpc, vpcAttachment) alone — identical for a veth master's host end and a tap master's tap device, so no interface-kind inference is needed the way galactic-bgp needs it. It requires a non-nil prevResult and passes it through unchanged.

cmdDel is a no-op, matching the pre-split behavior: the old monolithic plugin's own DEL never deleted termination routes either — they're keyed by (vpc, vpcAttachment) and may still be in use by another pod/VM, so cleanup stays with galactic-router's GC controller. cmdCheck is checkTerminationRoutes, moved unchanged. cmdStatus is a trivial always-ready success, matching galactic-ipam's own STATUS.

Rollback scoping

resourceTracker covers only the termination routes this plugin's own ADD actually installed (route-delete only) — rollback never touches a route a failed route.Add call never reached.

Master plugin cleanup

internal/cni/internal/cnitap drop the Terminations field from their own PluginConf (each had its own copy of a Termination type — now lives only in cniroute), the route.Add loop and routesCreated tracker field, and the checkTerminationRoutes call from CHECK.

Verification

🤖 Generated with Claude Code

@privateip
privateip requested a review from a team as a code owner August 8, 2026 04:42
@privateip
privateip requested review from felixwidjaja and removed request for a team August 8, 2026 04:42
@privateip privateip mentioned this pull request Aug 8, 2026
Open
@privateip
privateip force-pushed the refactor/cni-chain-3-galactic-route branch from d48f088 to 7f0a73f Compare August 8, 2026 12:02
This was referenced Aug 8, 2026
Open
Open
Open
@privateip
privateip force-pushed the refactor/cni-chain-3-galactic-route branch from 7f0a73f to a1e29e7 Compare August 8, 2026 12:15
@privateip privateip mentioned this pull request Aug 8, 2026
Open
8 tasks
@privateip
privateip force-pushed the refactor/cni-chain-3-galactic-route branch from a1e29e7 to 96f0f09 Compare August 8, 2026 12:27
@privateip
privateip force-pushed the refactor/cni-chain-3-galactic-route branch from 96f0f09 to c1704f7 Compare August 8, 2026 12:37
@mattdjenkinson

mattdjenkinson commented Aug 8, 2026

Copy link
Copy Markdown

Reviewed this one. Overall it's a clean, careful mirror of the sibling packages (cnibgp, cnitap), the resource tracking, rollback, and prevResult handling all follow the established pattern. Two real bugs turned up, though, one of which will break at runtime despite tests and vet passing.

cmd/galactic-route/main.go:67 never sets CNI_NETNS_OVERRIDE, and that will fail ADD/DEL for every VM/tap-mode attachment that has terminations. For a VM/tap-mode attachment, CNI_NETNS is deliberately set to the host's own root netns since there's no per-VM netns to enter, and galactic-route runs natively in that same host netns. Both galactic-cni and galactic-tap-cni handle this today by setting CNI_NETNS_OVERRIDE=true in their own process before calling into the skel library. galactic-route skips that step. The vendored CNI skel dispatcher checks ns.CheckNetNS(cmdArgs.Netns) after the plugin's own Add/Del already ran, sees that the plugin's namespace equals CNI_NETNS, and returns ErrInvalidNetNS unless that override env var is set in its own process, which it isn't here since it's a separately exec'd binary and env doesn't propagate across the chain. So the route actually gets installed correctly, but the CNI runtime sees ADD/DEL reported as failed for every VM/tap-mode attachment with terminations. The doc comment's reasoning, that galactic-route "never enters any network namespace" and so needs neither netns handling nor the override, conflates entering a namespace with the ambient-namespace-equality check the library actually performs.

internal/cniroute/ops_check.go:82 rejects valid on-link routes in CHECK. checkTerminationRoutes runs net.ParseIP(term.Via) and treats a nil result as an error before ever looking at the actual route table. But Termination.Via is omitempty, and assembleRoute in route.go has a dedicated branch for an empty Via that installs a valid on-link route, device-scoped, no gateway. cmdAdd installs this fine. CHECK, though, always fails with "invalid termination gateway """ for any on-link termination even though the route is present and correct. This logic was carried over byte for byte from the pre-split internal/cni/ops_check.go, so the PR relocated an existing bug into galactic-route's CHECK implementation rather than introducing a new one.

Two lower-severity things. docs/cni/configuration.md:100 still documents "terminations" as a field of the master plugin's own JSON config, which this PR removes. An operator following the doc would put terminations inside a galactic-cni or galactic-tap-cni stanza instead of a galactic-route one; the now-slimmer PluginConf silently drops the unknown field on unmarshal, ADD succeeds with no routes installed, and the master's own CHECK no longer checks routes either since that was removed here, so nothing signals that anything's wrong. And internal/cniroute/config.go:205's if conf.PrevResult != nil branch in parseConf is dead code, since PluginConf.PrevResult has json tag "-" and is never populated by json.Unmarshal. Production stays safe because cmdAdd separately validates through parsePrevResult, but this exact pattern is copy-pasted from internal/cni, internal/cnitap, and internal/cnibgp, so it predates this PR and is more of a systemic cleanup item than a regression here.

The netns-override miss is the one I'd block on, it'll break every VM/tap workload with terminations despite the test suite being green. The CHECK bug on on-link routes is worth fixing too since it's a genuine functional bug, just inherited rather than introduced by this PR.

@privateip
privateip force-pushed the refactor/cni-chain-3-galactic-route branch from c1704f7 to dc3f41c Compare August 8, 2026 19:04
privateip added a commit that referenced this pull request Aug 9, 2026
@privateip @claude
fix(cnibgp): address PR #305 review feedback from mattdjenkinson
7c6fad1 
Rollback context (blocking): rollbackCtx was created up front with a
single 10s budget and reused for tracker.cleanup() after publishBGPState's
own retryK8sOps could already burn ~30s across its retries. A slow API
server could leave cleanup with an expired context, turning Delete's
"NotFound" into "context deadline exceeded" (which client.IgnoreNotFound
doesn't catch), leaking the just-created CRDs. rollbackCancel was also
only called on the error branch, leaking a timer on every successful ADD.
Now rollbackCtx is created fresh, with its own full budget, only inside
the failure branch.

CHECK coverage (blocking): checkEBPFEntry only read back vrf_table, so a
corrupted/missing locator_table or function_table entry, or a nodeID that
drifted out of range after ADD, still reported the attachment healthy.
It now also verifies locator_table, function_table, and the nodeID range,
matching what ADD treats as a hard error.

e2e coverage (blocking): TestCNITapInterface's comment claimed eBPF
registration happens inline in galactic-tap-cni's cmdAdd, which stopped
being true with the plugin-chain split, and the test never exercised
galactic-bgp at all -- silently losing coverage of BGP CRD creation and
eBPF registration on the ADD path. It now chains /galactic-bgp ADD (fed
the tap master's real result as prevResult) and CHECK after the tap
master's own ADD, asserting the BGPVRFInstance/BGPAdvertisement CRDs exist
and that CHECK -- which reads back all three eBPF tables -- passes.

cniVersion constraint: type100.NewResult only accepts "1.0.0"/"1.1.0",
and the master plugin echoes the conflist's own cniVersion straight into
its printed Result, so an older value fails galactic-bgp's ADD for every
attachment. Documented as an explicit, intentional requirement in
prevresult.go and docs/cni/configuration.md, with a test locking in the
rejection of older versions.

DEL version/logging: cmdDel never parsed args.StdinData, always printed
cniVersion "1.0.0" unconditionally (unlike internal/cni/ops_del.go, which
only falls back to that on a parse failure), and never logged
vpc/vpcAttachment. It now parses the conflist, logs the attachment, and
uses pluginConf.CNIVersion with the same fallback-on-parse-failure
pattern as the other DEL implementations in this chain.

Interface-kind inference: veth/tap was inferred purely from
len(Interfaces) (1 vs 2), enforced only in comments -- #306 chaining
galactic-route into this same prevResult could add a host-side interface
and silently misclassify tap as veth (or vice versa) instead of failing
loudly, since both counts are valid switch cases. inferFromPrevResult now
counts Sandbox-carrying interfaces instead: the actual property that
distinguishes veth (guest end moved into the container netns) from tap
(host-only), which survives an extra host-side interface without
misclassifying.

Smaller cleanup:
- Removed the stale bgpv1alpha1.AddToScheme registration (with a circular
  justifying comment) from internal/cni/resource.go and
  internal/cnitap/resource.go -- neither package touches BGP CRDs anymore.
- Added resource_test.go covering resourceTracker.cleanup's own wiring end
  to end (all three resource kinds, plus the rollback-collision race at
  that level) -- previously only the standalone unregisterEBPFDatapath was
  tested directly.
- resourceTracker now embeds publishResult instead of cmdAdd copying its
  five tracking fields over one by one, so a future field added to one
  can't silently stop being tracked in the other.
- Removed egressKindForInterfaceType's dead empty-string branch --
  inferFromPrevResult always produces "veth" or "tap" now.

Also added prevresult_test.go and ops_del_test.go, and introduced a
package-level ebpfPinDir var (defaulting to attach.PinDir) so tests can
redirect galactic-bgp's own eBPF registration/rollback/CHECK reads to a
throwaway pin directory instead of the real production one.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
privateip added a commit that referenced this pull request Aug 9, 2026
@privateip @claude
fix(cnibgp): address PR #305 review feedback from mattdjenkinson
7cf773a 
Rollback context (blocking): rollbackCtx was created up front with a
single 10s budget and reused for tracker.cleanup() after publishBGPState's
own retryK8sOps could already burn ~30s across its retries. A slow API
server could leave cleanup with an expired context, turning Delete's
"NotFound" into "context deadline exceeded" (which client.IgnoreNotFound
doesn't catch), leaking the just-created CRDs. rollbackCancel was also
only called on the error branch, leaking a timer on every successful ADD.
Now rollbackCtx is created fresh, with its own full budget, only inside
the failure branch.

CHECK coverage (blocking): checkEBPFEntry only read back vrf_table, so a
corrupted/missing locator_table or function_table entry, or a nodeID that
drifted out of range after ADD, still reported the attachment healthy.
It now also verifies locator_table, function_table, and the nodeID range,
matching what ADD treats as a hard error.

e2e coverage (blocking): TestCNITapInterface's comment claimed eBPF
registration happens inline in galactic-tap-cni's cmdAdd, which stopped
being true with the plugin-chain split, and the test never exercised
galactic-bgp at all -- silently losing coverage of BGP CRD creation and
eBPF registration on the ADD path. It now chains /galactic-bgp ADD (fed
the tap master's real result as prevResult) and CHECK after the tap
master's own ADD, asserting the BGPVRFInstance/BGPAdvertisement CRDs exist
and that CHECK -- which reads back all three eBPF tables -- passes.

cniVersion constraint: type100.NewResult only accepts "1.0.0"/"1.1.0",
and the master plugin echoes the conflist's own cniVersion straight into
its printed Result, so an older value fails galactic-bgp's ADD for every
attachment. Documented as an explicit, intentional requirement in
prevresult.go and docs/cni/configuration.md, with a test locking in the
rejection of older versions.

DEL version/logging: cmdDel never parsed args.StdinData, always printed
cniVersion "1.0.0" unconditionally (unlike internal/cni/ops_del.go, which
only falls back to that on a parse failure), and never logged
vpc/vpcAttachment. It now parses the conflist, logs the attachment, and
uses pluginConf.CNIVersion with the same fallback-on-parse-failure
pattern as the other DEL implementations in this chain.

Interface-kind inference: veth/tap was inferred purely from
len(Interfaces) (1 vs 2), enforced only in comments -- #306 chaining
galactic-route into this same prevResult could add a host-side interface
and silently misclassify tap as veth (or vice versa) instead of failing
loudly, since both counts are valid switch cases. inferFromPrevResult now
counts Sandbox-carrying interfaces instead: the actual property that
distinguishes veth (guest end moved into the container netns) from tap
(host-only), which survives an extra host-side interface without
misclassifying.

Smaller cleanup:
- Removed the stale bgpv1alpha1.AddToScheme registration (with a circular
  justifying comment) from internal/cni/resource.go and
  internal/cnitap/resource.go -- neither package touches BGP CRDs anymore.
- Added resource_test.go covering resourceTracker.cleanup's own wiring end
  to end (all three resource kinds, plus the rollback-collision race at
  that level) -- previously only the standalone unregisterEBPFDatapath was
  tested directly.
- resourceTracker now embeds publishResult instead of cmdAdd copying its
  five tracking fields over one by one, so a future field added to one
  can't silently stop being tracked in the other.
- Removed egressKindForInterfaceType's dead empty-string branch --
  inferFromPrevResult always produces "veth" or "tap" now.

Also added prevresult_test.go and ops_del_test.go, and introduced a
package-level ebpfPinDir var (defaulting to attach.PinDir) so tests can
redirect galactic-bgp's own eBPF registration/rollback/CHECK reads to a
throwaway pin directory instead of the real production one.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@privateip
refactor(cni): extract galactic-route as its own CNI chain plugin
b844261 
Step 3 of the CNI plugin-chain split (galactic/plan-cni-plugin-chain):
pulls termination-route installation out of the veth and tap master
plugins into its own chained CNI binary, galactic-route, invoked
between the master plugin and galactic-bgp per conflist order. Unlike
every other binary in the chain, galactic-route has zero Kubernetes
dependency — it neither reads nor writes any CRD, and never needs a
namespace.

internal/cniroute is the new plugin package, mirroring the shape
established by cniipam/cnibgp:
- cniroute.go: RunPlugin() entrypoint (skel.PluginMainFuncs, ADD/DEL/
  CHECK/STATUS/VERSION).
- types.go/config.go: PluginConf{VPC, VPCAttachment, Terminations},
  parsed from stdin — the same document the master plugin itself
  received, since the CNI runtime passes every chain entry its own
  stanza plus prevResult. parseConf still reuses config.CNIConfig for
  LogFile/LogLevel's env-var > conflist > default precedence (so
  logging behaves identically to every other binary), but — unlike
  galactic-bgp — never resolves NodeName or Kubeconfig, since nothing
  here ever talks to the API server.
- ops_add.go: cmdAdd installs each termination as a VRF route via the
  existing internal/cni/route package (route.Add), deriving the host
  device name from (vpc, vpcAttachment) alone via
  intf.GenerateInterfaceNameHost — identical for a veth master's host
  end and a tap master's tap device, so galactic-route needs no
  interface-kind inference the way galactic-bgp does. It then passes
  prevResult through unchanged, adding no interfaces or IPs of its
  own. Requires a non-nil prevResult (galactic-route must be chained
  after a master plugin) and reads it from RawPrevResult, not the
  never-populated typed PrevResult field.
- ops_del.go: cmdDel is a no-op, same as every other binary in the
  chain — termination routes are keyed by (vpc, vpcAttachment) and may
  still be in use by another pod/VM sharing the same attachment, so
  cleanup is left entirely to galactic-router's GC controller. This
  matches the pre-split behavior too: the old monolithic plugin's own
  DEL never deleted termination routes either, for the same reason —
  extracting this into its own binary changes nothing about when
  routes actually get removed.
- ops_check.go: cmdCheck is checkTerminationRoutes, moved unchanged
  from internal/cni/ops_check.go (also mirrored in internal/cnitap).
  cmdStatus is a trivial always-ready success — galactic-route has
  nothing external to probe, matching galactic-ipam's own STATUS,
  implemented for uniformity across the chain per the plan's decision
  rather than skipped.
- resource.go: a resourceTracker scoped to exactly what galactic-
  route's own ADD creates — the termination routes it actually
  installed (route-delete only). Rollback deletes only the routes
  recorded as added, never routes a failed route.Add call never
  reached.

internal/cni and internal/cnitap: dropped the Terminations field from
each PluginConf (both packages had their own copy of a Termination
type, now living only in cniroute since neither master plugin reads
"terminations" out of its own stanza anymore), the route.Add loop and
routesCreated tracker field from ops_add.go/resource.go, and the
checkTerminationRoutes call from ops_check.go's CHECK path (the
function itself moved to cniroute, verbatim).

Taskfile.yaml, containers/galactic-cni/Dockerfile, and
internal/installer/installer.go (SourceRouteBinary) gain galactic-route
alongside the four other chain binaries, following the exact pattern
established for those in steps 0-2.

Verification: task lint (0 issues), task build (all 8 binaries,
including galactic-route), task test:unit all green.
internal/cniroute lands at 62.9% coverage — its first-ever test
coverage, since internal/cni/route (the package it wraps) had none
before this split either; backfilling that package's own tests is
unrelated to this split's scope and left as-is. task test:e2e not run
in this step, same caveat as steps 0-2 (requires sudo modprobe vrf plus
a Kind cluster bring-up, deferred to the end of the full stack per the
plan's verification approach).
@privateip
privateip force-pushed the refactor/cni-chain-3-galactic-route branch from dc3f41c to b844261 Compare August 9, 2026 20:15
@privateip @claude
fix(cniroute): address PR #306 review feedback from mattdjenkinson
fe72a1e 
CNI_NETNS_OVERRIDE (blocking): galactic-route never entered any netns,
so cmd/galactic-route/main.go assumed it never needed the stdin
peek-and-repipe dance or CNI_NETNS_OVERRIDE that galactic-cni/
galactic-tap-cni use. That's true for veth-mode attachments, where
CNI_NETNS points at the container's netns and differs from this
process's own ambient (host) netns. It's false for tap-mode
attachments: CNI_NETNS is deliberately set to the host's own root
netns there (no per-VM netns exists), which equals this process's
ambient netns, so skel's post-Add/Del same-netns check rejected every
tap-mode ADD/DEL with terminations even though the route was already
installed correctly. Now peeks stdin for interface_type the same way
galactic-cni does and sets CNI_NETNS_OVERRIDE=true only for tap mode.

CHECK on-link routes: checkTerminationRoutes unconditionally called
net.ParseIP on Via and errored on nil, but Via is omitempty and
assembleRoute (route.go) has a real branch that installs a valid
on-link route for an empty Via, which cmdAdd installs fine. CHECK
always failed with "invalid termination gateway" for those regardless.
Restructured the match loop to treat an empty Via as looking for a
gateway-less, device-scoped route instead of erroring immediately.
Carried over byte for byte from the pre-split internal/cni/ops_check.go
(also reachable via internal/cnitap), so this fixes the same bug there
too by virtue of the code having moved.

Docs: docs/cni/configuration.md still listed terminations as a
galactic-cni/galactic-tap-cni field and showed it inline in the
master's own JSON, which this PR's PluginConf split made wrong --
the master's slimmer struct silently drops the field on unmarshal,
so an operator following the doc gets a silent no-op. Moved the field
out of the master's Top-Level Fields table, reworded Termination
Fields to attribute it to galactic-route's own conflist stanza
(including that cmdDel is a no-op, not "deleted in reverse order"),
and rewrote the worked example as a chained plugins array.

Deferred per the review: the internal/cniroute/config.go:205 dead
`if conf.PrevResult != nil` branch is copy-pasted across internal/cni,
internal/cnitap, and internal/cnibgp too, predating this PR -- left
for #307, which already scopes "dead code" cleanup for the chain
split.

Verification: task lint, task build (all 8 binaries), task test:unit
all pass. task test:e2e not run, same caveat as #303/#304/#305 --
this repo has no root/CAP_NET_ADMIN available, and the existing
checkTerminationRoutes tests already can't get past the vrf.TableID
lookup without a real kernel VRF, so the on-link CHECK fix has no new
automated regression test beyond what task test:e2e's Kind cluster
would exercise.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
privateip added a commit that referenced this pull request Aug 9, 2026
@privateip @claude
docs+fixes(cni): finish the CNI plugin-chain split — docs, containerl…
6fdfbda 
…ab, dead code

Rebased onto the updated refactor/cni-chain-3-galactic-route (which
picked up PR #306's own review-feedback fix commit, fe72a1e) after
that branch's history moved out from under this one.

Conflicts resolved:

- All 11 containerlab tenant NAD manifests: kept this PR's plugins-array
  chain wrapper (adding the galactic-bgp stage) around the nested
  "ipam": {"type": "galactic-ipam", ...} block that PR #305's own fix
  commit had already introduced independently of this PR -- the two
  changes were orthogonal, so the merge is additive.

- docs/cni/configuration.md: took this PR's fuller rewrite throughout
  (it supersedes PR #306's narrower doc fix -- e.g. this PR already
  covers the interface_type/terminations field removal and the
  renamed GALACTIC_IPAM_ENABLE_LOCAL_IPAM env var more completely),
  but preserved two things #306 fixed that this PR's diff predates and
  doesn't otherwise cover: the cniVersion 1.0.0/1.1.0 prevResult
  constraint paragraph, and "on-link route" (not "link-local route" --
  fd01::/48 in the example isn't a link-local address) in the
  terminations example.

- tests/e2e/e2e_test.go: this PR's diff removed startEBPFControlDaemon
  (call, definition, and the attach import) on the theory that
  TestCNITapInterface never touches the eBPF datapath. That was true
  when this PR's diff was authored, but PR #305's own fix commit
  (7cf773a) had independently added testChainedGalacticBGP, chaining
  galactic-bgp after the tap master's ADD and asserting BGPVRFInstance/
  BGPAdvertisement CRD creation -- and registerEBPFDatapath's
  usidmap.OpenPinnedRegistry only opens already-pinned maps, it never
  loads/pins the eBPF program itself, so testChainedGalacticBGP can't
  succeed without startEBPFControlDaemon having run first. Restored the
  call, its definition, and the import, and updated the doc comments
  this PR had already rewritten (which claimed the test "does not
  chain into ... galactic-bgp") to describe the merged reality instead.

Verification: task lint, task build (all 8 binaries), task test:unit
all pass on the rebased tree. tests/e2e not run in this sandbox (no
Kind cluster / root), same caveat as every PR in this stack.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
privateip added a commit that referenced this pull request Aug 9, 2026
@privateip @claude
fix(vrf): key kernel VRF by VPC, not (VPC, attachment)
e6e7a38 
Rebased onto the updated refactor/cni-chain-4-installer-docs (which
picked up PR #307's own review-feedback fix commit) after that
branch's history moved out from under this one -- same ripple as
#306 -> #307.

Conflicts resolved: internal/cni/resource.go and internal/cnitap/
resource.go both had two independent changes touching the same
struct/cleanup() region -- this PR's own removal of vrfCreated/VRF
deletion (since the VRF is now shared per-VPC, not per-attachment,
so a single attachment's rollback must never delete it), and PR
#305/#306's unrelated addition of ipamDelegated/ipamType/ipamStdin
for real IPAM-delegation rollback. Kept both: dropped vrfCreated and
the VRF-delete step, kept the IPAM rollback fields and step, and kept
this PR's fuller "why no VRF deletion" doc comment (it explains the
shared-VRF reasoning more completely than the version already in
these files). internal/cnibgp/bgp.go had one similar conflict at the
registerEBPFDatapath call site -- resolved to drop the vpcAttachment
argument (this PR's change) while keeping the ebpfPinDir package var
(added by #305's own fix, for test injection) rather than reverting
to the attach.PinDir literal this PR's diff predates.

Also fixed one file this PR's diff never touched: internal/cnibgp/
resource_test.go didn't exist yet at this PR's original base -- it
was added later by #305's own fix commit -- so its direct vrf.Add/
vrf.Delete/vrf.TableID(vpc, vpcAttachment) calls needed the same
vpcAttachment-arg removal this PR already applied everywhere else,
or the package wouldn't build.

Verification: task lint, task build (all 8 binaries), task test:unit
all pass on the rebased tree. tests/e2e not run in this sandbox (no
Kind cluster / root), same caveat as every PR in this stack.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
privateip added a commit that referenced this pull request Aug 9, 2026
@privateip @claude
docs+refactor(cni): fix stale package doc, promote hostconf/crdnames/…
49bc55f 
…nadpatch

Rebased onto the updated refactor/cni-chain-4-installer-docs (which
picked up PR #307's own review-feedback fix commit) after that
branch's history moved out from under this one -- same ripple as
#306 -> #307 -> #311.

Conflicts resolved: internal/cnibgp/bgp.go, ops_check.go, and
resource.go each had an import-block conflict from this PR's
internal/cni/crdnames -> internal/crdnames promotion landing on
lines the current tree had already changed independently (PR #305's
own fix commit moved cnibgp's eBPF pin-dir handling behind a package-
level ebpfPinDir var in cnibgp.go, so bgp.go/ops_check.go/resource.go
no longer import internal/plumbing/ebpf/attach directly the way this
PR's diff -- authored before that -- expected). Resolved by applying
just the crdnames rename to each file and leaving the attach import
out, matching how the current tree already gets ebpfPinDir.

Also fixed one file this PR's diff never touched: internal/cnibgp/
resource_test.go didn't exist yet at this PR's original base -- it
was added later by #305's own fix commit -- so its own
"go.datum.net/galactic/internal/cni/crdnames" import needed the same
promotion this PR already applied everywhere else, or the package
wouldn't build.

Verification: task lint, task build (all 8 binaries), task test:unit
all pass on the rebased tree, including the three promoted packages
(internal/crdnames, internal/hostconf, internal/nadpatch). tests/e2e
not run in this sandbox (no Kind cluster / root), same caveat as
every PR in this stack.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
privateip added a commit that referenced this pull request Aug 9, 2026
@privateip @claude
fix(cnibgp): share BGPVRFInstance and Argument per (VPC, node)
8dd8bef 
Rebased onto the updated fix/vrf-shared-per-vpc-1-core (which picked
up my own rebase-and-reconcile of that branch after PR #307 moved
out from under it) after that branch's history moved out from under
this one -- same ripple as #306 -> #307 -> #311 -> #312.

Conflicts resolved: this PR drops the eBPF vrf_table
rollback path (registerEBPFDatapath no longer returns a block to
track, unregisterEBPFDatapath is deleted entirely, publishResult
loses ebpfRegistered/ebpfBlock/ebpfArgument) since the vrf_table
entry is now shared per (VPC, node) same as the BGPVRFInstance CRD,
so a failed ADD must never unregister it. The current tree had
independently refactored resourceTracker to embed publishResult
(rather than copying its fields one-by-one) between this PR's
original base and now, so I kept that embedding -- it still holds
exactly this PR's two surviving fields (advertisementCreated,
vrfInstanceCreated) once the eBPF fields are gone -- and added this
PR's own vrfInstanceCreated-conditioned-on-OperationResultCreated
behavior and nodeName field on top of it, along with this PR's fuller
cleanup() doc comment (it explains the shared-VRF reasoning more
completely than what was already there). Also kept ebpfPinDir (the
package-level var #305's own fix added for test injection) over the
attach.PinDir literal this PR's diff predates, matching the same
resolution #311 needed one level up.

internal/cnibgp/resource_test.go needed a full rewrite rather than a
per-hunk merge: it didn't exist yet at this PR's original base either
(same gap #311 hit) -- it was added by #305's own fix commit -- so
this PR's diff shows the file as "new," and the version already in
the tree still tested the old unconditional-vrfInstanceCreated/
ebpfRegistered design this PR removes. Took this PR's four tests
wholesale (they're purpose-built for the new design) and adjusted
their resourceTracker literals for the embedded-publishResult shape
(publishResult: publishResult{vrfInstanceCreated: true} instead of a
bare vrfInstanceCreated: true field, which the embedding makes
illegal in a keyed literal).

Verification: task lint, task build (all 8 binaries), task test:unit
all pass on the rebased tree. tests/e2e not run in this sandbox (no
Kind cluster / root), same caveat as every PR in this stack.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
privateip added a commit that referenced this pull request Aug 9, 2026
@privateip @claude
fix(containerlab): give ns30/ns40 two attachments, not one shared
02ed3a1 
Rebased onto the updated fix/vrf-shared-per-vpc-2-cnibgp (which
picked up my own rebase-and-reconcile of that branch after
fix/vrf-shared-per-vpc-1-core moved out from under it) after that
branch's history moved out from under this one -- same ripple as
#306 -> #307 -> #311 -> #312 -> #313.

No conflicts: this PR only touches containerlab docs/scripts/
manifests, none of which overlapped with the Go source changes further
up the stack. Applied cleanly.

Verification: task lint, task build (all 8 binaries) pass on the
rebased tree. No Go source changed in this PR, so task test:unit is
unaffected. tests/e2e and the containerlab lab itself not run in this
sandbox (no Kind cluster / root), same caveat as every PR in this
stack.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
privateip added a commit that referenced this pull request Aug 9, 2026
@privateip @claude
refactor(cni): dedupe internal/cni vs internal/cnitap into internal/c…
8213e7b 
…nimaster

Rebased onto the updated fix/cni-review-followups-doc-placement
(which picked up my own rebase-and-reconcile of that branch after PR
#307 moved out from under it) after that branch's history moved out
from under this one -- same ripple as #306 -> #307 -> #315 -> #316.

Conflicts resolved: internal/cni/resource.go and internal/cnitap/
resource.go each had two independent changes touching the same
resourceTracker/cleanup() region -- this PR's own extraction of the
shared k8s-client-construction (newK8sClient/cniScheme) and interface+
VRF rollback (veth.Delete/tap.Delete + vrf.Delete) into
internal/cnimaster's NewK8sClient/CleanupAttachment, and PR #305/#306's
unrelated addition of ipamDelegated/ipamType/ipamStdin fields plus an
IPAM-release rollback step (this PR's diff predates that feature
entirely, same gap #311/#312/#315 each hit one level up). Kept both:
call cnimaster.CleanupAttachment for the interface+VRF half (this PR's
whole point), and kept the IPAM release step ahead of it, unchanged.
internal/cnitap/ops_check.go had one similar import-only conflict
(this PR drops the netlink/rest/ctrl/vrf imports cnimaster.
CheckNodeLevelState/ProbeAPIServer/RunStatus now cover internally) --
its own IPAM CHECK delegation step (ipam.ExecCheck, same predates-this-
PR gap) sat entirely outside the conflicted hunk and needed no
resolution beyond keeping the "github.com/containernetworking/plugins/
pkg/ipam" import alive.

Verification: task lint, task build (all 8 binaries), task test:unit
all pass on the rebased tree, including the new internal/cnimaster
package. tests/e2e not run in this sandbox (no Kind cluster / root),
same caveat as every PR in this stack.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@ecv

ecv commented Aug 9, 2026

Copy link
Copy Markdown

@privateip see matt's comment?

@ecv ecv mentioned this pull request Aug 9, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@felixwidjaja felixwidjaja Awaiting requested review from felixwidjaja felixwidjaja is a code owner automatically assigned from datum-cloud/engineering
@0xmc 0xmc Awaiting requested review from 0xmc
@scotwells scotwells Awaiting requested review from scotwells
@slindseysr slindseysr Awaiting requested review from slindseysr

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@privateip @mattdjenkinson @ecv